Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add method find_both_related() #1633

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: add method find_both_related() #1633

wants to merge 6 commits into from

Conversation

ProbablyClem
Copy link
Contributor

@ProbablyClem ProbablyClem commented May 8, 2023 •
edited
Loading

PR Info

New Features

  • new selector SelectBoth in which the return type is Vec<(E::Model, F::Model)>
  • a new method fn select_both<F>(mut self, _: F) -> SelectBoth<E, F> on Select
  • new method fn find_both_related()<R>(self, r: R) -> SelectBoth<E, R>

@ProbablyClem ProbablyClem changed the title feat: add method inner_join_and_select feat: add method inner_join_and_select May 8, 2023
@Pascualex
Copy link

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

@ProbablyClem
Copy link
Contributor Author

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

Thanks @Pascualex
Yeah, I do like that name, as it matches the find_xxx_related as talked in the issue.
Do you think I should rename the inner_join_and_select or create a new one @tyt2y3 ?

@ProbablyClem
Copy link
Contributor Author

I renamed it for now, as I think find_both_related is a better name.
I can still go back to inner_join_and_select if wanted just let me know

@ProbablyClem ProbablyClem changed the title feat: add method inner_join_and_select feat: add method find_both_related() May 18, 2023
tyt2y3
tyt2y3 reviewed May 20, 2023
View reviewed changes
src/query/join.rs Outdated Show resolved Hide resolved
tyt2y3
tyt2y3 approved these changes May 20, 2023
View reviewed changes
Copy link
Member

@tyt2y3 tyt2y3 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, find_both_related is a good name.

@tyt2y3 tyt2y3 requested a review from billy1624 May 20, 2023 14:48
@ProbablyClem ProbablyClem mentioned this pull request Aug 6, 2023
Closed
@Bolognafingers
Copy link

Shame this wasn't merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyt2y3 tyt2y3 tyt2y3 approved these changes

@billy1624 billy1624 Awaiting requested review from billy1624

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

find_also_related() with non null relation, or using inner join, should not return Option
4 participants
@ProbablyClem @Pascualex @Bolognafingers @tyt2y3